[core-xml] Create @azure/core-xml#10307
Conversation
Also fix some tests that weren't ported correctly.
xirzec
added
Client
xirzec
requested review from
KarishmaGhiya,
bterlson,
mikeharder and
ramya-rao-a
as code owners
|
|
||
| let errorNS = ""; | ||
| try { | ||
| errorNS = parser.parseFromString("INVALID", "text/xml").getElementsByTagName("parsererror")[0] |
There was a problem hiding this comment.
What's the goal of this block? as I understand, this is trying to figure the error structure, which may vary depending on the browser. Once that is figured out errorNS can be used to check if there are any errors.
Is the reason this is on a block by itself and not inside throwIfError, that we just want to run this (expensive?) figuring out once?
There was a problem hiding this comment.
Found the PR that introduced this line of code, but seems like it was grabbing the error NS before: https://github.com/Azure/ms-rest-js/pull/211/files
Setting the variable once has been in there since this support was originally added: Azure/ms-rest-js#187
Perhaps @RikkiGibson could enlighten us?
There was a problem hiding this comment.
This was the best way I found at the time to discover the namespace used to report XML parsing errors, and therefore find out whether the return value of parseFromString represents a parse failure. I wrote it to discover the errorNS eagerly at startup because it's simple, and you need it every time you try to parse XML anyway. I can't speak to whether e.g. computing it from scratch each time or lazy initializing it is better.
There was a problem hiding this comment.
Thanks for the context, I think it's probably fine to do this eagerly, especially now that this code isn't even loaded as part of the main http pipeline and will only show up in services that need XML support.
| assert.exists(response); | ||
| assert.isUndefined(response.readableStreamBody); | ||
| assert.isUndefined(response.blobBody); | ||
| assert.isUndefined(response.parsedBody); |
There was a problem hiding this comment.
I am curious what leads to this change?
There was a problem hiding this comment.
When I converted the tests I rewrote some of the assertions... and because many test assertions are similar I was copy pasting a lot. Since the tests were disabled though I didn't notice my copypaste errors. 😅
5 participants
Per the core 2.0 work, this change creates a new package called
@azure/core-xmlthat handles all XML parsing and serialization. This allows generated clients to conditionally pull in thexml2jsdependency.Fixes #8616